-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fixes issues with the System.Net.Http tests #1975
Conversation
mmitche
commented
Jun 9, 2015
- Tests were not running because the directories were not named properly.
- Rename test project to match the correct name.
- Fix typo in resource strings.
<data name="net_http_client_send_canceled" xml:space="preserve"> | ||
<value>Request for {0} was canceled.</value> | ||
<data name="net_http_client_send_cancelled" xml:space="preserve"> | ||
<value>Request for {0} was cancelled.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a misprint. It was explicitly spelled that way (with one "L"). There are two accepted spellings of the word "canceled". It turns out that much of the .NET Framework is inconsistent on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay will revert this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We've tried to standardized on one 'l' (except in "cancellation", which has two 'l's), e.g. "OperationCanceledException". There was some usage in the framework that predated that, though.
@dotnet-bot test this please |
@stephentoub Was this to retest after project.lock.json fixes? If so, I think I have to merge to head and push again. |
It was to re-test after I checked in #1974. |
@stephentoub yeah, this will just retest the old merge. Let me remerge first. |
Looks like 10 tests are failing on Linux, mostly related to globalization. |
@mmitche Are you looking at the test failures for this PR? |
I took a quick look at them in the CI report; I believe they're all related to #846 and should simply be marked as [ActiveIssue(846, PlatformID.AnyUnix)] to suppress their execution on Linux and OSX. Most of them are due to a lack of support for string normalization, and one has to do with a lack of proper encoding support. |
@stephentoub How did the tests pass before this PR? I.e., when the System.Net.Http tests were first added, the PR build was good. |
I believe the primary issue Matt is fixing here is that the tests weren't actually being run then, or at least weren't being run on Linux, because of the scripting expecting a specific directory structure that this library didn't match. |
Yes that's the case. These may need to be disabled on Linux for now. |
ok. Once you fix the tests (or disable them), then LGTM. |
@davidsh I'm having trouble getting the build to reference xunit.netcore.extensions (contains the ActiveIssue attribute) without floating the set of packages used with System.Net.Http. The newest packages appear to require use of the newest dnx.exe, I've PR'd the update to DNX as #2052 and am waiting on that to test again. |
@davidsh Currently blocked on the dnx update |
@dotnet-bot test this please |
@davidsh I might have to do some tweaks, not exactly sure what state I left this branch. Let me give it a quick try and we'll see how it goes. |
b6a0a02
to
7657a51
Compare
* Tests were not running because the directories were not named properly. * Rename test project to match the correct name. * Fix typo in resource strings.
7657a51
to
36b6d6b
Compare
@mmitche The build finished successfully. So, I think this can be merged now. |
(thank goodness :)) Yes. |
Fixes issues with the System.Net.Http tests
Fixes issues with the System.Net.Http tests Commit migrated from dotnet/corefx@df881ff